Skip to content
This repository has been archived by the owner on Nov 15, 2023. It is now read-only.

Collectives: teleport slashed assets #1433

Merged
merged 11 commits into from
Jul 21, 2022
Merged

Collectives: teleport slashed assets #1433

merged 11 commits into from
Jul 21, 2022

Conversation

muharem
Copy link
Contributor

@muharem muharem commented Jul 7, 2022 •
edited
Loading

Teleport slashed assets to the Polkadot treasury account.

Passes integration tests paritytech/parachains-integration-tests#26

@muharem muharem added A0-pleasereview B0-silent Changes should not be mentioned in any release notes labels Jul 7, 2022
@joepetrowski joepetrowski mentioned this pull request Jul 7, 2022
Merged
5 tasks
muharem
muharem commented Jul 7, 2022
View reviewed changes
Cargo.lock Outdated Show resolved Hide resolved
@xlc
Copy link
Contributor

xlc commented Jul 7, 2022

This sounds like a bad idea that makes DoS relaychain much easier. Why not just sent to local treasury and have another method to teleport local treasury to parent treasury.

gilescope
gilescope approved these changes Jul 8, 2022
View reviewed changes
Copy link
Contributor

@gilescope gilescope left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice work!

@joepetrowski
Copy link
Contributor

This sounds like a bad idea that makes DoS relaychain much easier.

How? The DoS protection is the fact that someone is getting slashed. And the slasher in this case is the Polkadot Alliance, which is semi-trusted. If the Alliance is DoSing the network then we have bigger problems.

Why not just sent to local treasury and have another method to teleport local treasury to parent treasury.

  1. Because there is no local treasury.
  2. The Alliance members should not be in control of the tokens.
  3. There should be a clear story about what happens when someone is slashed and where the tokens go. Putting them in limbo just adds confusion about the consequences of getting slashed and more work for others to sort it out later.

@xlc
Copy link
Contributor

xlc commented Jul 13, 2022

I guess if slash operation is protected it will be fine. Still, having fun trying to figure out the correct weight for extrinsic that triggers slash.

By local treasury I mean just a local pallet account that is only accessible by root. Then relaychain governance can use XCM to teleport those funds back if needed. There is no need to have a treasury pallet.

The slash operation and amount is very clearly defined so I guess current approach is fine. However image if somehow resulting the slash amount to be less than ED, the cost of sending XCM is just not worth the teleport. This approach is unsafe as a generic way to handle slash. My only worry is in future, some new introduction of the slash mechanism could result unexpected complications.

@joepetrowski
Copy link
Contributor

The slash operation and amount is very clearly defined so I guess current approach is fine. However image if somehow resulting the slash amount to be less than ED, the cost of sending XCM is just not worth the teleport. This approach is unsafe as a generic way to handle slash. My only worry is in future, some new introduction of the slash mechanism could result unexpected complications.

I don't think this is a huge concern. If slashes are less than the ED then they are not really that strong of a disincentive.

This was referenced Jul 20, 2022
Merged
Merged
@muharem
Copy link
Contributor Author

muharem commented Jul 21, 2022

Integration tests for the use case - paritytech/parachains-integration-tests#26
PR to allow teleport to the westend relay chain - paritytech/parachains-integration-tests#26

@muharem muharem requested a review from NachoPal July 21, 2022 13:16
@joepetrowski joepetrowski merged commit 6087ae8 into joe-alliance Jul 21, 2022
@joepetrowski joepetrowski deleted the collectives-slash-assets-handler branch July 21, 2022 14:47
paritytech-processbot bot pushed a commit that referenced this pull request Aug 11, 2022
@joepetrowski @gilescope
@muharem @alvicsam @chevdor @KiChjang
Add Collectives Parachain (#1346)
0e83ad2 
* add new runtime and remove unnecessary pallets

* make runtime build

* add collectives to collator node

* sketch alliance config in runtime

* Slash handler was supposed to be commented out (for now)

* correct signature

* move to impls

* add alliance to runtime

* rustfmt

* IsReserve, remove Ping, update fn deposit

* add transaction_payment event

* Update parachains/runtimes/collectives/collectives-polkadot/src/lib.rs

Co-authored-by: Squirrel <gilescope@gmail.com>

* fmt

* add genesis config to chain spec

* fix merge

* local and dev configs only (for now)

* remove duplicate imports

* Collectives polkadot runtime to cargo workspace members (#1397)

* Collectives polkadot runtime: use unit type impl for identity verifier (#1398)

* apply fn rename

* fmt

* one less todo

* Less code in magic macros (#1407)

* Less code in magic macros

* cargo fmt

* Bench alliance (#1427)

* add benchmarks

* call one script from the other

* shebang changes so works on nixos too.

* bench in parallel as separate jobs

* hyphens can turn into underscores

* remove workaround to trigger bench

Co-authored-by: alvicsam <alvicsam@gmail.com>
Co-authored-by: paritytech-ci <paritytech-ci@parity.io>

* enable ci jobs

* fix publish bench results jobs

* chainspecs for collectives-westend (#1441)

* initial chainspecs for collections relay chain

* plumb in the collectives-westend chainspec

* add Runtime::CollectivesWestend

* lock

* Collectives: teleport slashed assets  (#1433)

* Collectives: teleport slashed assets

* fmt

* Cargo.lock > polkadot-parachain 0.9.25

* create temp account for imbalance

* treasury acc id from pallet id

* move accounts into constants, use here junction for assets

* assets location is relay chain, accounts as parameters

* fix typos

* fix typo

* Update parachains/runtimes/collectives/collectives-polkadot/src/constants.rs

Co-authored-by: joe petrowski <25483142+joepetrowski@users.noreply.github.com>

* Move alliance proposal provider to impls.rs (#1464)

* Move to impls alliance proposal provider

* rustfmt

* Bumping spec version

(so that we can redeploy with slashing change.)

* cargo lock

* slurp collectives digest to make appear in release notes (#1473)

* add slurp

* Slurp better :)

* Bring some order

Co-authored-by: Chevdor <chevdor@users.noreply.github.com>
Co-authored-by: Wilfried Kopp <wilfried@parity.io>

* reorder barrier

* Update parachains/runtimes/collectives/collectives-polkadot/src/xcm_config.rs

Co-authored-by: Keith Yeung <kungfukeith11@gmail.com>

* address review

* clean construct runtime

* fmt

* looks pretty but brings in too much

Co-authored-by: Squirrel <gilescope@gmail.com>
Co-authored-by: Muharem Ismailov <ismailov.m.h@gmail.com>
Co-authored-by: alvicsam <alvicsam@gmail.com>
Co-authored-by: paritytech-ci <paritytech-ci@parity.io>
Co-authored-by: Chevdor <chevdor@users.noreply.github.com>
Co-authored-by: Wilfried Kopp <wilfried@parity.io>
Co-authored-by: Keith Yeung <kungfukeith11@gmail.com>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@gilescope gilescope gilescope approved these changes

@joepetrowski joepetrowski joepetrowski approved these changes

@KiChjang KiChjang Awaiting requested review from KiChjang

@NachoPal NachoPal Awaiting requested review from NachoPal

Assignees
No one assigned
Labels
B0-silent Changes should not be mentioned in any release notes
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@muharem @xlc @joepetrowski @gilescope @KiChjang